Delete splash screen widget early #605

hebasto · 2022-05-22T17:57:39Z

Fixes #604.
Fixes bitcoin/bitcoin#25146.
Fixes bitcoin/bitcoin#26340.

SplashScreen::deleteLater() does not guarantee deletion of the m_splash object prior to the wallet context deletion. If the latter happens first, the segfault follows.

luke-jr · 2022-05-22T22:50:44Z

Why is this so much more complicated than bitcoin/bitcoin#25146 (comment) ?

hebasto · 2022-05-23T06:11:01Z

Why is this so much more complicated than bitcoin/bitcoin#25146 (comment) ?

Because this change makes the resulted code simpler.

src/qt/bitcoin.cpp

hebasto · 2022-05-23T08:48:22Z

Updated 66646b6 -> ec8d6aa (pr605.01 -> pr605.02, diff):

dropped unneeded condition

ryanofsky · 2022-05-23T15:13:15Z

I think I have a similar question to luke. With this PR Now there are two different manual deletions for wallet handler in two different places. The BitcoinApplication::requestShutdown() method has:

    // Delete wallet controller here manually, instead of relying on Qt object
    // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
    // walletmodel m_handle_* notification handlers are deleted before wallets
    // are unloaded, which can simplify wallet implementations. It also avoids
    // these notifications having to be handled while GUI objects are being
    // destroyed, making GUI code less fragile as well.
    delete m_wallet_controller;
    m_wallet_controller = nullptr;

And the BitcoinApplication::initializeResult method has:

    // If success, m_splash is no longer needed.
    // Otherwise, it must be deleted prior to requesting shutdown
    // to make sure the `SplashScreen::m_connected_wallet_handlers`
    // is deleted before the wallet context is.
    delete m_splash;
    m_splash = nullptr;

Shouldn't both wallet handler deletions be consolidated together? Or is there something that makes them different from each other?

hebasto · 2022-05-23T17:37:08Z

With this PR Now there are two different manual deletions for wallet handler in two different places.

Correct. But indirectly, via deleting of objects, members of which are wallet handlers.

Shouldn't both wallet handler deletions be consolidated together? Or is there something that makes them different from each other?

Those wallet handlers belong to very different objects, m_wallet_controller and m_splash, with different roles and life cycles.

OTOH, the suggested approach eliminates duplication of Q_EMIT splashFinished(), removes a signal, a slot, and two connections, and makes code less spaghetti-like.

ryanofsky · 2022-05-23T18:20:39Z

I agree this the approach in this PR is clean and simplifies code. I think I'm just wondering if it is sufficient to only delete the splash screen object in initializeResult, or if it the splash screen should also be deleted in requestShutdown? Will initializeResult always happen before requestShutdown? Also wondering why requestShutdown is the right place to delete the wallet controller.

The comments do a good job of explaining why it is important to delete these objects, but it's unclear why the objects are deleted in these places. It's possible there aren't concrete reasons, but I'm asking to learn if there are, and maybe improve the comments.

furszy · 2022-05-24T20:23:51Z

Wouldn't make sense to unsubscribe from the core signals before the deleteLater call so the object can be freed whenever QT wants without any fear of receiving a post-controller-deletion signal?

In other words, calling SplashScreen::unsubscribeFromCoreSignals() right before the deleteLater() call inside SplashScreen::finish(), or in this latest changes, change the delete for a deleteLater and call the unsubscribeFromCoreSignals just before it.

hebasto · 2022-05-25T15:26:08Z

I agree this the approach in this PR is clean and simplifies code. I think I'm just wondering if it is sufficient to only delete the splash screen object in initializeResult, or if it the splash screen should also be deleted in requestShutdown? Will initializeResult always happen before requestShutdown? Also wondering why requestShutdown is the right place to delete the wallet controller.

The comments do a good job of explaining why it is important to delete these objects, but it's unclear why the objects are deleted in these places. It's possible there aren't concrete reasons, but I'm asking to learn if there are, and maybe improve the comments.

The universal entry point of the shutdown process is BitcoinApplication::requestShutdown() which handles both internal and OS-initiated cases, and it always runs in the "main" GUI thread. That is why it is the only right place to delete the wallet controller (considering the current implementation of wallet handlers).

OTOH, BitcoinApplication::initializeResult() is called via Qt::QueuedConnection from the "qt-init"/"shutoff" thread. Therefore, there are no guarantees that it always happens before BitcoinApplication::requestShutdown() (for example, in case when the latter is triggered by OS's QEvent::Quit).

ryanofsky · 2022-05-25T17:47:12Z

re: #605 (comment)

OTOH, BitcoinApplication::initializeResult() is called via Qt::QueuedConnection from the "qt-init"/"shutoff" thread. Therefore, there are no guarantees that it always happens before BitcoinApplication::requestShutdown() (for example, in case when the latter is triggered by OS's QEvent::Quit).

So if QEvent::Quit happens, and BitcoinApplication::requestShutdown() runs before BitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?

If so, it seems like maybe we need to add delete m_splash; m_splash = nullptr; lines to both of the requestShutdown() and initializeResult() methods, not just one of the methods. Or is this not the case?

re: #605 (comment)

Wouldn't make sense to unsubscribe from the core signals before the deleteLater call so the object can be freed whenever QT wants without any fear of receiving a post-controller-deletion signal?

Unclear if it would make things more complicated to partially shut down wallet controller and splash screen objects by disconnecting them from signals, and let Qt delete them later, than to simply delete these objects ourselves when we know they are no longer needed. But maybe it would be more targeted and not actually more complicated. Qt does seem to not really care whether we delete the objects or it will. Both approaches seem like they would be ok.

hebasto · 2022-05-25T22:07:46Z

So if QEvent::Quit happens, and BitcoinApplication::requestShutdown() runs before BitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?

Correct, although this scenario is unlikely. Nevertheless, the code is flawed.

If so, it seems like maybe we need to add delete m_splash; m_splash = nullptr; lines to both of the requestShutdown() and initializeResult() methods, not just one of the methods. Or is this not the case?

Would it cleaner to introduce a dedicated SplashScreen::releaseWalletHandler() member function, and leave m_splash object lifetime management to Qt framework? The same approach could be used for wallet controller.

hebasto · 2022-05-26T14:28:33Z

So if QEvent::Quit happens, and BitcoinApplication::requestShutdown() runs before BitcoinApplication::initializeResult(), is there another bug still after this PR where the wallet is deleted before the splash screen, and there is a crash because the splash screen is still registered for wallet notifications?

Added a commit to prevent such a scenario.

ryanofsky

Code review ACK d4227f7, but I tend to think it would be good to keep the first three commits, drop the fourth commit "Ignore QEvent::Quit until full initialization", and add a new commit with achow's suggestion also deleting the splash screen in requestShutdown() so requestShutdown() is less fragile. (See comments below)

Would it cleaner to introduce a dedicated SplashScreen::releaseWalletHandler() member function, and leave m_splash object lifetime management to Qt framework? The same approach could be used for wallet controller.

IMO it is cleanest just to delete the splash screen as soon as you don't need the splash screen, rather than keep the splash screen alive but remove the wallet handler underneath it. If it were actually helpful to the user to keep showing the splash screen while unloading, maybe a more complicated approach like this could be good, but otherwise it seems like it wouldn't improve things.

ryanofsky · 2022-06-01T19:35:05Z

src/qt/bitcoin.cpp

 bool BitcoinApplication::event(QEvent* e)
 {
-    if (e->type() == QEvent::Quit) {
+    if (m_initialized && e->type() == QEvent::Quit) {


In commit "qt: Ignore QEvent::Quit until full initialization" (d4227f7)

This seems like a pretty indirect way of preventing requestShutdown() from running before initializeResult() so splashscreen will be deleted before the wallet and won't hold dangling references to the wallet.

It also seems pretty fragile, because this is just one requestShutdown() caller when there could be other callers now or in the future that fail to delete the splash screen before calling requestShutdown.

Also the commit seems to make code more complicated. There are no comments to explain the logic, and the commit message just has a vague message about requestShutdown being error prune, not saying why it is (or should be) error prone.

I think a better approach here would just to make the requestShutdown method delete splash screen and wallet at the same time, so requestShutdown calls are not error-prone and don't need to be avoided like this. Something like

delete m_splash; m_splash = nullptr; delete m_wallet_controller; m_wallet_controller = nullptr;

in requestShutdown like achow originally proposed bitcoin/bitcoin#25146 (comment) would make sense I think.

Additionally, it does seem good to delete the splash screen in initializeResult as you are doing in your first commit "qt: Delete splash screen widget early" (c7d2458), since that also makes sense and simplifiies code. In both cases I think it's good to delete the splash screen as soon as the splash screen is no longer needed.

DrahtBot · 2022-06-12T21:44:46Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, dooglus, john-moffett
Stale ACK	ryanofsky, achow101, jarolrod

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

hebasto · 2022-10-24T10:01:13Z

Updated d4227f7 -> d2cfc7d (pr605.03 -> pr605.04):

rebased
addressed @ryanofsky's comments

drop the fourth commit "Ignore QEvent::Quit until full initialization", and add a new commit with achow's suggestion also deleting the splash screen in requestShutdown() so requestShutdown() is less fragile

Done.

cc @achow101 @dooglus @furszy @luke-jr @promag @ryanofsky

achow101 · 2022-10-24T17:17:27Z

Code Review ACK d2cfc7d

jarolrod

tACK d2cfc7d

I have reviewed the code and I agree it can be merged. Additionally did testing where I quit in various scenarios where the splash screen was still loading and quiting afterwards as well.

In relation to bitcoin/bitcoin#26340, I still couldn't recreate the issue. But since the splash screen would be the only core window showing at this time, it's reasonable to think it could be related.

furszy · 2022-10-25T13:19:23Z

src/qt/bitcoin.cpp

 #ifdef ENABLE_WALLET
-    // Delete wallet controller here manually, instead of relying on Qt object
+    // Delete splash screen and wallet controller here manually, instead of relying on Qt object
    // tracking (https://doc.qt.io/qt-5/objecttrees.html). This makes sure
    // walletmodel m_handle_* notification handlers are deleted before wallets
    // are unloaded, which can simplify wallet implementations. It also avoids
    // these notifications having to be handled while GUI objects are being
    // destroyed, making GUI code less fragile as well.
    delete m_wallet_controller;
    m_wallet_controller = nullptr;
+    delete m_splash;
+    m_splash = nullptr;


The deletion should be placed outside of the #ifdef ENABLE_WALLET scope.
As GUI uses the splash screen even when it runs without a wallet (--disable-wallet), the object will not be deleted if a shutdown is requested during load.

the object will not be deleted if a shutdown is requested during load.

~~It will, via Qt parent-child relation.~~

Early widget deletion is crucial when wallet code is active only.

the object will not be deleted if a shutdown is requested during load.

It will, via Qt parent-child relation.

The SplashScreen has no parent. It's a standalone dialog (check the view constructor).

You can try it by adding a print inside the splash screen destructor and call requestShutdown at any time during load (blocking initializeResult execution).

This ensures that during shutdown, including failed initialization, the `SplashScreen::m_connected_wallet_handlers` is deleted before the wallet context is.

furszy

ACK 1b22849
The failing CI task is pure bad luck.

dooglus · 2022-11-06T20:13:24Z

~~This fix causes a crash elsewhere: bitcoin/bitcoin#26340 (comment)~~

ACK 1b22849

john-moffett

ACK 1b22849

hebasto added the Bug Something isn't working label May 22, 2022

hebasto mentioned this pull request May 22, 2022

Starting with an unsupported wallet configured leads to a segfault (master only?) bitcoin/bitcoin#25146

Closed

hebasto added the Wallet label May 22, 2022

promag reviewed May 23, 2022

View reviewed changes

src/qt/bitcoin.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 220522-splash branch from 66646b6 to ec8d6aa Compare May 23, 2022 08:44

ryanofsky approved these changes Jun 1, 2022

View reviewed changes

hebasto mentioned this pull request Oct 24, 2022

bitcoin-qt crashes if it can't listen on the port it wants to bitcoin/bitcoin#26340

Closed

hebasto force-pushed the 220522-splash branch from d4227f7 to d2cfc7d Compare October 24, 2022 09:55

DrahtBot mentioned this pull request Oct 24, 2022

multiprocess: Add bitcoin-gui -ipcconnect option bitcoin/bitcoin#19461

Draft

jarolrod approved these changes Oct 25, 2022

View reviewed changes

furszy reviewed Oct 25, 2022

View reviewed changes

hebasto added 3 commits October 27, 2022 12:58

qt: Delete splash screen widget explicitly

5299cfe

This ensures that during shutdown, including failed initialization, the `SplashScreen::m_connected_wallet_handlers` is deleted before the wallet context is.

qt: Drop no longer used BitcoinApplication::splashFinished() signal

10811af

qt: Drop no longer used SplashScreen::finish() slot

1b22849

hebasto force-pushed the 220522-splash branch from d2cfc7d to 1b22849 Compare October 27, 2022 12:29

furszy approved these changes Oct 27, 2022

View reviewed changes

john-moffett approved these changes Dec 7, 2022

View reviewed changes

hebasto merged commit 497f265 into bitcoin-core:master Dec 20, 2022

hebasto deleted the 220522-splash branch December 20, 2022 20:14

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 21, 2022

Merge bitcoin-core/gui#605: Delete splash screen widget early

a638d1b

bitcoin-core locked and limited conversation to collaborators Dec 20, 2023

Delete splash screen widget early #605

Delete splash screen widget early #605

Uh oh!

Conversation

hebasto commented May 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented May 22, 2022

Uh oh!

hebasto commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented May 23, 2022

Uh oh!

hebasto commented May 23, 2022

Uh oh!

ryanofsky commented May 23, 2022

Uh oh!

furszy commented May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 25, 2022

Uh oh!

ryanofsky commented May 25, 2022

Uh oh!

hebasto commented May 25, 2022

Uh oh!

hebasto commented May 26, 2022

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 24, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jun 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented Oct 24, 2022

Uh oh!

achow101 commented Oct 24, 2022

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy Oct 25, 2022

Choose a reason for hiding this comment

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dooglus commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

hebasto commented May 22, 2022 •

edited

Loading

hebasto commented May 23, 2022 •

edited

Loading

hebasto commented May 23, 2022 •

edited

Loading

furszy commented May 24, 2022 •

edited

Loading

ryanofsky Jun 1, 2022 •

edited

Loading

DrahtBot commented Jun 12, 2022 •

edited

Loading

hebasto Oct 25, 2022 •

edited

Loading

furszy left a comment •

edited

Loading

dooglus commented Nov 6, 2022 •

edited

Loading